feat: implement adaptive frequency detection and auto-parameter adjustment#285
Conversation
- Add sampling_frequency property with setter to IMUConfig - Implement _update_frequency_dependent_params() virtual method - Override in GaitConfig: auto-clamp spectrum_high_frequency and mfcc_high_frequency to Nyquist - Override in TremorConfig: auto-clamp fmax_peak_search and fmax_mfcc to Nyquist - Add warnings when frequency bounds are clamped for safety - Fixes issue where 50Hz/64Hz data caused 'out of bounds' errors in feature extraction This allows users to set sampling_frequency at any point and freq-dependent params adapt automatically, preventing Nyquist violations without manual config management.
…ss initialization This fixes the initialization order bug where the update method was being called in IMUConfig.__init__() before subclass attributes like mfcc_high_frequency were set. Solution: defer calls until the end of GaitConfig and TremorConfig __init__. The property setter still works for post-initialization frequency changes, and the virtual method pattern correctly updates all bounds when sampling_frequency changes at any point in the config lifecycle. Verified with test_adaptive_frequencies_demo.py - all 3 tests pass.
- Remove test_adaptive_frequencies_demo.py (development validation only) - Remove implementation notes from IMUConfig (internal architectural detail) - Keep core adaptive frequency bounds implementation clean for production Commit validates that all 3 frequency test cases (50/64/100 Hz) pass successfully.
…fix-adaptive-frequency-bounds
|
Couple of notes:
|
There was a problem hiding this comment.
If I understand it correctly, the resampling frequency is set to 100 Hz if it is not provided, but this will result in upsampling of the data if the sampling frequency is below 100 Hz right? Shouldn't it be set to the auto-detected sampling frequency to ensure uniform sampling at that frequency?
…Hz when resampling
Yes, good catch! I think I've changed it correctly now. I've additionally removed resampling from data preparation (in Note that I've now added segmentation to data preparation as this functionality was called inside the |
- Add import for numpy and segmenting module functions - Implement auto-segmentation in prepare_raw_data() using create_segments() and discard_segments() - This restores the ability to split non-contiguous data into segments during data preparation - Auto-segmentation is independent of resampling and operates on time gaps - Update docstring to clarify auto-segmentation functionality - All tests now pass
- duration_for_categorization_s: unfiltered parent segment duration - duration_s: actual segment data duration (filtered or unfiltered) Filtered arm swing of 5s in 25s unfiltered segment now correctly: - Categorizes into 20+s (based on parent unfiltered duration) - Sums only 5s to category total (actual filtered data)
… branch Changed duration_for_categorization_s back to duration_unfiltered_segment_s for consistency with release branch naming conventions. This maintains clear structure where both filtered and unfiltered aggregations use the same field for categorization (unfiltered parent segment duration = 25s for 0-20s/20+ determination), while duration_s varies per dataset type (5s for filtered, 25s for unfiltered).
Keep only segment_category for aggregation logic, aligned with category-only approach. The unfiltered duration is no longer stored since categorization is pre-computed and stored in segment_category field. This field will be reintroduced if needed during backward compatibility fallback implementation.
| - **Gyroscope**: `deg/s` (degrees per second) | ||
|
|
||
| > ParaDigMa has functionalities for converting [accelerometer](https://biomarkersparkinson.github.io/paradigma/autoapi/paradigma/util/index.html#paradigma.util.convert_units_accelerometer) and [gyroscope](https://biomarkersparkinson.github.io/paradigma/autoapi/paradigma/util/index.html#paradigma.util.convert_units_gyroscope) in other units (e.g., `m/s^2`, `rad/s`) to these standard units. This can also be setup when using `run_paradigma`. | ||
| > ParaDigMa has functionalities for converting [accelerometer](https://biomarkersparkinson.github.io/paradigma/autoapi/paradigma/util/index.html#paradigma.util.convert_units_accelerometer) and [gyroscope](https://biomarkersparkinson.github.io/paradigma/autoapi/paradigma/util/index.html#paradigma.util.convert_units_gyroscope) in other units (e.g., `m/s^2`, `rad/s`) to these standard units. This can also be setup when using run_paradigma`. |
There was a problem hiding this comment.
Is the ` missing? Or should you also remove the latter one?
|
|
||
| > [!NOTE] | ||
| > The specifications above represent **minimally validated requirements**. For example, while ParaDigMa works with accelerometer and gyroscope data sampled at 50 Hz, the effect on processing accuracy has not been empirically validated. | ||
| > The specifications above represent **validated requirements**. ParaDigMa has been tested and verified to work correctly with sampling frequencies as low as 50 Hz, with comparable accuracy to 100 Hz baseline. |
There was a problem hiding this comment.
If you state that it has been tested and verified to work correctly then the part after the comma becomes redundant right?
| """ | ||
| Update frequency-dependent parameters when sampling_frequency changes. | ||
|
|
||
| Ensures that spectral bounds stay within Nyquist limit (fs/2). |
There was a problem hiding this comment.
Maybe a minor semantic thing but technically it is called Nyquist frequency right?
KarsVeldkamp
left a comment
There was a problem hiding this comment.
In general this is looking good, but I have a couple of questions before ready to merge
| df=df, | ||
| segment_nr_colname="data_segment_nr", | ||
| min_segment_length_s=( | ||
| min_segment_length_s if min_segment_length_s is not None else 1.5 |
| logger.info(f"Created {n_segments} segments") | ||
|
|
||
| # Deprecation notice for resampling_frequency parameter | ||
| if resampling_frequency != 100.0: |
There was a problem hiding this comment.
What if it is set at 100? Maybe not the most logical thing to do but then the user still expect to already have resampled data right? Or am I not understanding this correctly?
This ensures that the tremor pipeline works well for odd sampling frequencies, since rounding the number of overlapping samples resulted in the creation of too many subwindows
|
@Erikpostt I have made adjustments to the tremor pipeline so that odd sampling frequencies work well too. |
There was a problem hiding this comment.
Should we change this to sampling_frequency = int(1/current_dt)? The tremor pipeline currently relies on the sampling frequency being an integer, but not sure where to adjust this
|
I've made some additional changes after sitting down with Kars. In summary: Documentation updates
Config
Pipelines updated accordingly Preprocessing
@nienketimmermans am I correct that EDIT: this needs a bit more work and testing. Let's discuss this later. |
|
@nienketimmermans if we do automatic resampling, the test data is actually resampled to 101 Hz (instead of 100 Hz) causing quite some changes. Could you check the markdown files and inspect these changes? Perhaps we should be more lenient on rounding (e.g., use only even sampling rates or something similar). What do you think? We can discuss on Monday if needed. |
…ng a resmapling_frequency is not recommended
nienketimmermans
left a comment
There was a problem hiding this comment.
Ready to merge!
|
Thank you @nienketimmermans; only awaiting @KarsVeldkamp for approval (upon his return from leave). |
|
@Erikpostt The preprocess_ppg_data() function currently modifies the ppg_config object (and similar for imu_config in my own and your pipelines) by calling config._set_sampling_frequency_detected() internally. While this works, for me it makes the intent unclear to callers: it is not obvious that the config object will be mutated. I am not sure what if this is an issue at all but maybe we can discuss this further. This is my only issue with the current adjustments, so apart from this we are ready to merge |
…mpling_frequency; Clarified updating of config parameters after resampling;
|
@KarsVeldkamp I've made some changes based on our discussion. Can you verify these changes and perhaps provide some additional suggestions if needed? |
Description
This PR implements the foundation for adaptive frequency detection and automatic parameter adjustment across ParaDigMa pipelines. This is based on making ParaDigMa work for lower sampling frequencies, where we noticed that upsampling is inferior to keeping the same frequency but adjusting frequency-based features accordingly.
The changes should not break / change much.
Changes
detect_sampling_frequency()utility with median-based detectionsampling_frequencyproperty setter with auto-update of frequency-dependent parametersresampling_frequencyoptional (maintains current frequency by default)Key Features
✅ Backward compatible with existing code